-
Notifications
You must be signed in to change notification settings - Fork 483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handling ini files #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -22,6 +32,14 @@ def __init__(self, charset, limit, *args): | |||
# TODO: Update for not just python comments? | |||
self.ignore_regex = re.compile(r'# ?pragma: ?whitelist[ -]secret') | |||
|
|||
def analyze(self, file, filename): | |||
# Heuristically determine whether file is an ini-formatted file. | |||
for ext in INI_FILE_EXTENSIONS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could maybe run file
on them and see if it comes back ASCII text
, might get expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ini
styled formats is definitely a subset of ASCII text
, so that solution may lead to false positives.
Eg.
$ file test_data/config.ini
test_data/config.ini: ASCII text
$ file test_data/baseline.file
test_data/baseline.file: ASCII text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of ambivalent, right now it's easier to exclude files than include them in the tuple, which makes me lean towards including more by default/making the false-p/false-n rate customizable. Nothing is really set in stone though, we can always change it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Let me jiggle it up a bit.
Login to GH and GHE to fix deploy
First part of fix for #12.
I currently just assume that config files in this format end with
.ini
. Technically, they can end with anything, but maybe we can just keep adding toINI_FILE_EXTENSIONS
?